Firestore Swift Cpp Experiment #13956
Conversation
Generated by 🚫 Danger |
Apple API Diff ReportCommit: 732555b [BUILD ERROR] FirebaseFirestoreFirebaseCoreFunctions[MODIFIED] FIRFirebaseVersion[MODIFIED] FIRFirebaseVersionSwift:
+ func FirebaseVersion () -> String
- func FirebaseVersion () -> String
Objective-C:
+ NS_SWIFT_NAME ( FirebaseVersion ()) NSString * FIRFirebaseVersion ( void )
- NSString * _Nonnull FIRFirebaseVersion ( void )[BUILD ERROR] FirebaseFirestoreInternal |
cherylEnkidu
force-pushed
the
cheryllin/firestoreSwiftCpp
branch
from
4a5f5fd to
7c7ac2e
Compare
FirebaseFirestore.podspec
Outdated
| s.public_header_files = 'FirebaseFirestoreInternal/**/*.h' | ||
| s.public_header_files = [ | ||
| 'FirebaseFirestoreInternal/**/*.h', | ||
| 'Firestore/Swift/Source/SwiftAPI/*.swift', |
There was a problem hiding this comment.
Swift files should not be in the headers
cherylEnkidu
force-pushed
the
cheryllin/firestoreSwiftCpp
branch
from
4af3d5b to
497cbf8
Compare
cherylEnkidu
force-pushed
the
cheryllin/firestoreSwiftCpp
branch
from
6265d7b to
e880583
Compare
| @@ -0,0 +1,43 @@ | |||
| // swift-tools-version:5.9 | |||
There was a problem hiding this comment.
I thought setting swift-tools-version is only meaningful for Package.swift files? What does it mean here?
| @discardableResult | ||
| public func GetPipelineResult() async throws -> [PipelineResult] { | ||
| return try await withCheckedThrowingContinuation { continuation in | ||
| let listener = CallbackWrapper.wrapPipelineCallback(firestore: cppObj.GetFirestore()) { |
There was a problem hiding this comment.
What needs to be done to fix the CII issues highlighted below?
There was a problem hiding this comment.
Looks like the CI cannot pick up the changes in the public Objective-C API, not sure why ...
| @@ -1392,6 +1398,52 @@ func firestoreWrapperTarget() -> Target { | |||
| ) | |||
| } | |||
|
|
|||
| func firebaseFirestoreCppTarget() -> Target { | |||
There was a problem hiding this comment.
Why an additional target versus modifying the existing Firestore target?
Will pipelines be an optional feature to be configured into the SDK?
There was a problem hiding this comment.
Firestore currently has three public folders for C++, Objective-C, and Swift.
Each build target can only specify a single path for the public header folder.
Ideally, the structure should follow the pattern: Swift wrapping Objective-C, which wraps C++. However, reorganizing the existing structure to achieve this is complex. Additionally, future improvements can still be made without requiring changes to the public API. For these reasons, I have chosen to add another target as a temporary workaround.
| + (std::unique_ptr<core::EventListener<std::vector<api::PipelineResult>>>) | ||
| wrapPipelineCallback:(std::shared_ptr<api::Firestore>)firestore | ||
| completion:(void (^)(std::shared_ptr<std::vector<api::PipelineResult>> result, | ||
| NSError *_Nullable error))completion |
There was a problem hiding this comment.
Is this intended to be exposed to customers? And should it use the ObjC types for it's parameters instead of C++ ones? e.g. FIRFirestore over std::shared_ptr<api::Firestore>
There was a problem hiding this comment.
The reason for using C++ types instead of Objective-C is that this class is intended to be used exclusively internally by the Swift layer and not by developers. The goal is to minimize the use of Objective-C wherever possible.
|
|
||
| + (std::unique_ptr<core::EventListener<std::vector<api::PipelineResult>>>) | ||
| wrapPipelineCallback:(std::shared_ptr<api::Firestore>)firestore | ||
| completion:(void (^)(std::shared_ptr<std::vector<api::PipelineResult>> result, |
There was a problem hiding this comment.
This is a newer learning, but it seems like making the callbacks in callback-based APIs Sendable so they can be invoked on any thread can prevent crashes when they do get called on a different thread. If this callback may be called on a different thread than the one it was defined in, then we should mark it as Sendable with.
| completion:(void (^)(std::shared_ptr<std::vector<api::PipelineResult>> result, | |
| completion:(void (^NS_SWIFT_SENDABLE)(std::shared_ptr<std::vector<api::PipelineResult>> result, |
Aw I see, so public headers can only used relative path? The reason I am asking is that I cannot include existing files from the previous API folder, Also, use relative path would fail syntax check: |
Here are some imperfections in this PR. Please feel free to suggest better solutions:
Exposure of C++ APIs in the Objective-C Interface
The Swift layer can only access public APIs of Objective-C. In Swift Package Manager, only a single path can be designated as the public header path. To enable additional features in the Swift layer, it must access internal C++ members held by the Objective-C object.
Swift's async/await functionality requires Objective-C callbacks for implementation. As a result, a public API—used exclusively by the Swift layer—was introduced. (Potentially this can be avoid using a C++ class contains Swift member, since the current structure only supports one way dependency, Swift class has C++ member. This impl is listed as a future improvement)
The FirebaseFirestoreCpp binary build structure in Swift Package Manager is currently written as pseudo-code and has not been verified.
The include path in this PR using relative path instead of full path, since full path leads to build failure for unknown reason.